Skip to content

Declare constants properly #403

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 10, 2022
Merged

Declare constants properly #403

merged 1 commit into from
Nov 10, 2022

Conversation

@armanbilge
Copy link

@armanbilge armanbilge commented Nov 5, 2022

A constant value definition is of the form

final val x = e

where e is a constant expression. The final modifier must be present and no type annotation may be given. References to the constant value x are themselves treated as constant expressions; in the generated code they are replaced by the definition's right-hand side e.

https://www.scala-lang.org/files/archive/spec/2.13/04-basic-declarations-and-definitions.html#value-declarations-and-definitions

cquiroz
cquiroz reviewed Nov 8, 2022
View reviewed changes
core/shared/src/main/scala/org/threeten/bp/Duration.scala

/** Constant for nanos per second. */
private def NANOS_PER_SECOND: Int = 1000000000
private final val NANOS_PER_SECOND = 1000000000
Copy link
Owner

@cquiroz cquiroz Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure these don't affect js size?

Copy link
Author

@armanbilge armanbilge Nov 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to reason about this.

Previously, every time this constant was used it would require a function call to NANOS_PER_SECOND. This requires a function NANOS_PER_SECOND to be emitted in the JS. This also means it's harder for the JS runtime to optimize it.

Now the generated JS code will never have NANOS_PER_SECOND in it. This is because before emitting JS code the Scala compiler will replace all the uses directly with 1000000000.

cquiroz
cquiroz approved these changes Nov 8, 2022
View reviewed changes
Copy link
Owner

@cquiroz cquiroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good for the most part. I'm not sure if the changes from val to def or viceversa may have a small impact on js size

@armanbilge armanbilge closed this Nov 10, 2022
@armanbilge armanbilge reopened this Nov 10, 2022
@cquiroz cquiroz merged commit c8825b8 into cquiroz:master Nov 10, 2022
@som-snytt
Copy link

som-snytt commented Nov 10, 2022 •
edited
Loading

Apparently, it is hard to reason about this. I was curious what a literal type gives you.

class C {
  final val x = 42
  final val y: 42 = 42

  def f = x
  def g = y
  def f2 = x+1
  def g2 = y+1
}

Scala 3 correctly does the same for both, which is no field.

Scala 2 incorrectly or suboptimally has field for y and accessor is invoked for g, but g2 is constant-folded (during type checking).

I did not try JS but will someday.

@som-snytt som-snytt mentioned this pull request Nov 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cquiroz cquiroz cquiroz approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@armanbilge @som-snytt @cquiroz